-
Notifications
You must be signed in to change notification settings - Fork 7.9k
pdo_odbc: allocate sufficient space for retrieving unicode data #15008
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: PHP-8.2
Are you sure you want to change the base?
Conversation
@jnahmias The test file name is gh9498_1.phpt. Are you planning on adding multiple tests? If there is only one, it may be better to remove the The other changes look good to me, but I'll wait for other people's opinions. edit: |
FWIW this is kinda similar to what ibm_db2 and friends do with their https://github.com/php/pecl-database-ibm_db2/blob/master/ibm_db2.c#L1991-L2005 |
@SakiTakamachi: Yes, I have another test planned for the INSERT path; see phpGH-9498 for details; However, from my review it seems to be a deeper issue that is only somewhat related (and probably should be a separate issue) so I have not made it part of this PR. Happy to rename it without the suffix if that's desired. |
Work around the fact that Windows does not use UTF-8 by doing the comparison within the PHP test code. Also, add a few more tests of plain ASCII strings to make sure those aren't impacted by this change. Fixes: e053712
Hmm, I'm really unsure how to solve this issue, but I think regardless of the exact solution, we should add more common routines for ext/odbc and ext/pdo_odbc to main/php_odbc_utils. Otherwise we likely end up solving similar issues twice. Comming back to the issue at hand: how does ext/odbc behave in this regard? |
Yeah, somewhat similar but rather different (see Also somewhat related to PR #10809. |
Yeah, that old PR did solve some problems I had that were similar, although I was still unsure if it was the right solution, especially as it might have had some performance impact. |
Closes GH-9498
cc: @NattyNarwhal @cmb69